fix(cas): fall back from unwritable installation pools - #349
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds cached storage writability probing, writable installation CAS selection, legacy-pool retention, fallback CAS lookup, and centralized installation-pool setup. Content services now delegate pool configuration to the installation CAS pool service. ChangesCAS pool writability and migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
Bug
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs (1)
615-647: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck the result of
EnsurePoolPathAsyncbefore proceeding.
EnsureInstallationPoolPathAsyncawaitsinstallationCasPoolService.EnsurePoolPathAsyncat Line 641 but discards the returned boolean. InContentOrchestrator.cs, the same call captures the result and fails the operation when it isfalse(Lines 552-557 of that file). Here, if pool-path resolution fails (for example, no writable pool can be established for any detected installation),DeliverContentAsyncstill proceeds to callmanifestPool.AddManifestAsyncfor theGameClientmanifest at Line 339, so content can be registered without a confirmed writable effective pool.This contradicts the PR objective to ensure all installation-related content types resolve to a writable effective pool before storage. Change the method to return the success flag, and let the caller in
DeliverContentAsyncfail fast, consistent withContentOrchestrator.EnsureInstallationPoolPathAsync.🐛 Proposed fix to propagate pool-path resolution failure
- private async Task EnsureInstallationPoolPathAsync(CancellationToken cancellationToken) + private async Task<bool> EnsureInstallationPoolPathAsync(CancellationToken cancellationToken) { try { // ALWAYS force installation detection and reset the path // Even if a path is set, it might be stale (from before user deleted data) // or point to the wrong installation logger.LogInformation("Forcing installation detection to ensure correct InstallationPoolRootPath"); installationService.InvalidateCache(); // Get all installations (this will trigger detection if cache is empty) var installationsResult = await installationService.GetAllInstallationsAsync(cancellationToken); if (!installationsResult.Success || installationsResult.Data == null) { logger.LogWarning("Failed to get installations for CAS pool path resolution: {Error}", installationsResult.FirstError); - return; + return false; } var installations = installationsResult.Data.ToList(); if (installations.Count == 0) { logger.LogWarning("No installations detected - cannot set InstallationPoolRootPath"); - return; + return false; } - await installationCasPoolService.EnsurePoolPathAsync(installations, cancellationToken); + return await installationCasPoolService.EnsurePoolPathAsync(installations, cancellationToken); } catch (Exception ex) { logger.LogError(ex, "Failed to ensure InstallationPoolRootPath is set"); + return false; } }And at the call site:
var hasGameClientManifest = manifests.Any(m => m.ContentType == ContentType.GameClient); if (hasGameClientManifest) { - await EnsureInstallationPoolPathAsync(cancellationToken); + var poolPathReady = await EnsureInstallationPoolPathAsync(cancellationToken); + if (!poolPathReady) + { + return OperationResult<ContentManifest>.CreateFailure( + "Could not ensure a writable InstallationPoolRootPath for GameClient content."); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs` around lines 615 - 647, Update EnsureInstallationPoolPathAsync to return the boolean result from installationCasPoolService.EnsurePoolPathAsync, returning false on failed installation lookup, no installations, or caught exceptions. In DeliverContentAsync, check this result before adding the GameClient manifest and fail fast when pool-path resolution is unsuccessful, matching the existing ContentOrchestrator behavior.GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/ContentOrchestratorTests.cs (1)
91-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
GameClientpool-path branch.
AcquireContentAsync_ValidatesAndStoresContent_Successfullyuses a manifest with the defaultContentType, so it never exercises the new branch at Lines 550-558 ofContentOrchestrator.csthat calls_installationCasPoolService.EnsurePoolPathAsync. Add a test wheremanifest.ContentType == ContentType.GameClientand_installationCasPoolServiceMockreturnsfalse, and assert thatAcquireContentAsyncreturns a failure result without callingAddManifestAsync. This closes a gap called out in the PR objectives for reinitialization and unwritable-pool scenarios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/ContentOrchestratorTests.cs` around lines 91 - 146, Add a dedicated GameClient acquisition test alongside AcquireContentAsync_ValidatesAndStoresContent_Successfully, configure manifest.ContentType to ContentType.GameClient and _installationCasPoolServiceMock.EnsurePoolPathAsync to return false, then assert AcquireContentAsync returns failure and _manifestPoolMock.AddManifestAsync is never called. Keep the existing successful default-content test unchanged.GenHub/GenHub/Common/Services/ConfigurationProviderService.cs (1)
320-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the default configuration from
Clone()to stop field drift.The new properties are copied correctly. The surrounding projection still duplicates
CasConfiguration.Clone()by hand. Each new CAS property must now be added in two places, andGcLockTimeoutis already missing here, so this branch silently resets it to the default. UseClone()and override onlyCasRootPath.♻️ Proposed refactor
- return new CasConfiguration - { - CasRootPath = defaultPath, - InstallationPoolRootPath = casConfig.InstallationPoolRootPath, - IsInstallationPoolRootPathAutoDerived = casConfig.IsInstallationPoolRootPathAutoDerived, - LegacyInstallationPoolRootPath = casConfig.LegacyInstallationPoolRootPath, - EnableAutomaticGc = casConfig.EnableAutomaticGc, - HashAlgorithm = casConfig.HashAlgorithm, - GcGracePeriod = casConfig.GcGracePeriod, - MaxCacheSizeBytes = casConfig.MaxCacheSizeBytes, - AutoGcInterval = casConfig.AutoGcInterval, - MaxConcurrentOperations = casConfig.MaxConcurrentOperations, - VerifyIntegrity = casConfig.VerifyIntegrity, - }; + var defaultConfig = (CasConfiguration)casConfig.Clone(); + defaultConfig.CasRootPath = defaultPath; + return defaultConfig;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub/Common/Services/ConfigurationProviderService.cs` around lines 320 - 333, Update the CasConfiguration projection in the relevant ConfigurationProviderService method to create the default configuration via casConfig.Clone(), then override only CasRootPath with defaultPath. Remove the manual property-by-property copying so fields such as GcLockTimeout remain preserved automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@GenHub/GenHub.Core/Interfaces/Common/IStorageWritabilityProbe.cs`:
- Around line 8-13: Update the XML documentation for
IStorageWritabilityProbe.CanCreateStorageAt to explicitly state that a
successful check may create and leave the storage directory on disk. Clarify
that callers should account for this side effect, especially when probing
read-only paths such as CasPoolResolver.GetLegacyInstallationPoolRootPath.
In
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/CasPoolWritabilityTests.cs`:
- Around line 153-162: Update
StorageWritabilityProbe_WhenLocationIsWritable_LeavesNoProbeFile to build the
Directory.GetFiles search pattern from StorageConstants.WriteProbeFilePrefix
instead of a hardcoded probe prefix, ensuring the assertion detects leaked
files. Also replace the ".genhub-cas" literals at the referenced setup and test
locations with DirectoryNames.GenHubCasPool, adding the GenHub.Core.Constants
import if needed.
In
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/InstallationCasPoolServiceTests.cs`:
- Line 42: Update the poolPath setup in the affected installation CAS pool tests
to use the shared DirectoryNames.GenHubCasPool constant instead of the hardcoded
".genhub-cas" literal, so assertions follow the same contract as
InstallationCasPoolService.GetDerivedPoolPath.
- Around line 232-236: Update Dispose in the test class to make
temporary-directory cleanup resilient when CasStorage or CasPoolManager leaves
Windows file handles open: catch cleanup-related IOException and
UnauthorizedAccessException from Directory.Delete(_tempPath, true) so teardown
does not mask the test result, while retaining GC.SuppressFinalize(this).
In `@GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs`:
- Around line 219-237: Replace the hand-written CasConfiguration projection in
GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs lines 219-237 within
CreateStorage with a clone of _config, then override CasRootPath with rootPath.
Apply the same change in
GenHub/GenHub/Common/Services/ConfigurationProviderService.cs lines 320-333
within GetCasConfiguration: clone casConfig and override CasRootPath with
defaultPath, preserving all configuration properties including GcLockTimeout.
- Around line 239-264: Refactor RefreshInstallationPools so GetStorage does not
acquire _initLock or call Directory.Exists on every lookup. Add an unlocked fast
pre-check comparing the current resolver roots with _installationPoolRoot and
_legacyInstallationPoolRoot, entering the existing locked refresh only when
roots change or cached state requires initialization; cache the legacy-root
availability and re-evaluate it through ReinitializeInstallationPool instead of
RefreshLegacyInstallationPool on every call.
- Around line 118-128: Update GetAllStorages to read _legacyInstallationStorage
once into a local variable after RefreshInstallationPools, then use that local
for the null check, containment check, and add operation. Do not access the
shared field again in this method.
In `@GenHub/GenHub/Features/Storage/Services/CasPoolResolver.cs`:
- Around line 79-89: Update the legacy-path branch in the resolver method
containing LegacyInstallationPoolRootPath to return the configured path only
when it is non-empty and Directory.Exists passes; otherwise continue to the
existing InstallationPoolRootPath fallback logic. Keep the current writability
validation and empty-string behavior for the current path unchanged.
In `@GenHub/GenHub/Features/Storage/Services/CasService.cs`:
- Line 577: Change the legacy CAS pool hit logging in both loops within
CasService to use LogDebug instead of LogInformation, including the comparable
log statement around the existing line 558 fallback path, while preserving the
message and hash argument.
- Around line 626-642: Update the fallback loop in ExistsAsync to skip both the
already-checked primaryStorage and the current storage, matching the exclusion
logic in GetContentPathAsync. Preserve fallback checks for all other storages
and the existing exists/break behavior.
In `@GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs`:
- Around line 170-173: Update the installationPath handling in
InstallationCasPoolService so it removes the final path segment only when the
path points to an existing file, rather than using the lexical Path.HasExtension
check. Preserve directory paths containing dots unchanged so pool derivation and
IsAutoDerived receive the correct installation directory.
- Around line 38-70: Update EnsurePoolPathAsync in InstallationCasPoolService so
the branches for no installations, no usable preferred-installation path, and
invalid normalized derived path return true, matching their logged fallback to
the primary CAS pool. Preserve false only when saving settings fails, and leave
the successful derived-pool path unchanged.
- Around line 206-216: Update SelectLegacyPath and
CasConfiguration.LegacyInstallationPoolRootPath to retain a collection of legacy
installation roots rather than a single path. During migration, append newly
discovered valid roots without replacing previously retained roots, and update
legacy-reader and read-only lookup consumers to search the full collection in
order. Preserve existing behavior when no additional legacy root is available.
- Around line 75-88: The historical auto-derived marker in EnsurePoolPathAsync
must be tracked separately from ExplicitlySetProperties. Update the migration
logic around historicalAutoDerivedMarker and IsAutoDerived so an explicitly
user-configured installation pool path is never classified as auto-derived,
replaced, or moved to LegacyInstallationPoolRootPath; remove or clear the
migration marker after it is used.
In `@GenHub/GenHub/Infrastructure/DependencyInjection/ConfigurationModule.cs`:
- Around line 51-52: Remove the explicit StorageWritabilityProbe logger
registration that uses bootstrapLoggerFactory, allowing its
ILogger<StorageWritabilityProbe> dependency to resolve through the main
AddLoggingModule logging pipeline.
---
Outside diff comments:
In
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/ContentOrchestratorTests.cs`:
- Around line 91-146: Add a dedicated GameClient acquisition test alongside
AcquireContentAsync_ValidatesAndStoresContent_Successfully, configure
manifest.ContentType to ContentType.GameClient and
_installationCasPoolServiceMock.EnsurePoolPathAsync to return false, then assert
AcquireContentAsync returns failure and _manifestPoolMock.AddManifestAsync is
never called. Keep the existing successful default-content test unchanged.
In `@GenHub/GenHub/Common/Services/ConfigurationProviderService.cs`:
- Around line 320-333: Update the CasConfiguration projection in the relevant
ConfigurationProviderService method to create the default configuration via
casConfig.Clone(), then override only CasRootPath with defaultPath. Remove the
manual property-by-property copying so fields such as GcLockTimeout remain
preserved automatically.
In
`@GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs`:
- Around line 615-647: Update EnsureInstallationPoolPathAsync to return the
boolean result from installationCasPoolService.EnsurePoolPathAsync, returning
false on failed installation lookup, no installations, or caught exceptions. In
DeliverContentAsync, check this result before adding the GameClient manifest and
fail fast when pool-path resolution is unsuccessful, matching the existing
ContentOrchestrator behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f8cabc53-af28-467d-86ea-4fd6c9947d18
📒 Files selected for processing (20)
GenHub/GenHub.Core/Interfaces/Common/IStorageWritabilityProbe.csGenHub/GenHub.Core/Interfaces/Storage/ICasPoolResolver.csGenHub/GenHub.Core/Interfaces/Storage/IInstallationCasPoolService.csGenHub/GenHub.Core/Models/Storage/CasConfiguration.csGenHub/GenHub.Tests/GenHub.Tests.Core/Common/Services/StorageLocationServiceTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/ContentOrchestratorTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/CasPoolWritabilityTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/InstallationCasPoolServiceTests.csGenHub/GenHub/Common/Services/ConfigurationProviderService.csGenHub/GenHub/Common/Services/StorageLocationService.csGenHub/GenHub/Common/Services/StorageWritabilityProbe.csGenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.csGenHub/GenHub/Features/Content/Services/ContentOrchestrator.csGenHub/GenHub/Features/Storage/Services/CasPoolManager.csGenHub/GenHub/Features/Storage/Services/CasPoolResolver.csGenHub/GenHub/Features/Storage/Services/CasService.csGenHub/GenHub/Features/Storage/Services/CasStorage.csGenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.csGenHub/GenHub/Infrastructure/DependencyInjection/CasModule.csGenHub/GenHub/Infrastructure/DependencyInjection/ConfigurationModule.cs
💤 Files with no reviewable changes (1)
- GenHub/GenHub/Features/Storage/Services/CasStorage.cs
| if (Path.HasExtension(installationPath)) | ||
| { | ||
| installationPath = Path.GetDirectoryName(installationPath); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Path.HasExtension misclassifies directories whose name contains a dot.
Path.HasExtension is a lexical check only. It returns true for real directory paths such as D:\Games\Command and Conquer Generals Zero Hour 1.04 or D:\Games\C&C.Generals. Line 172 then strips the game folder and returns the parent.
Two failures follow. The derived pool lands beside the game folder instead of inside it. Two installations under one parent, both with dots in the folder name, derive the same pool path. The wrong values also enter derivedPaths at lines 50-55, so IsAutoDerived at line 192 misclassifies a stored path.
The intent is to handle a path that points at an executable. Test the filesystem instead.
🐛 Proposed fix
- if (Path.HasExtension(installationPath))
- {
- installationPath = Path.GetDirectoryName(installationPath);
- }
+ // Detection may report an executable path; use its containing directory.
+ if (File.Exists(installationPath))
+ {
+ installationPath = Path.GetDirectoryName(installationPath);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Path.HasExtension(installationPath)) | |
| { | |
| installationPath = Path.GetDirectoryName(installationPath); | |
| } | |
| // Detection may report an executable path; use its containing directory. | |
| if (File.Exists(installationPath)) | |
| { | |
| installationPath = Path.GetDirectoryName(installationPath); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs` around
lines 170 - 173, Update the installationPath handling in
InstallationCasPoolService so it removes the final path segment only when the
path points to an existing file, rather than using the lexical Path.HasExtension
check. Preserve directory paths containing dots unchanged so pool derivation and
IsAutoDerived receive the correct installation directory.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /// <inheritdoc/> | ||
| public async Task<bool> EnsurePoolPathAsync( | ||
| IReadOnlyList<GameInstallation> installations, | ||
| CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
WARNING: cancellationToken is accepted but never honored
The token is declared on EnsurePoolPathAsync but never read, and IUserSettingsService.TryUpdateAndSaveAsync does not accept one. ContentOrchestrator and CommunityOutpostDeliverer forward their own cancellation token expecting cooperative cancellation, but the writability probe and settings save here run to completion regardless. Add a ThrowIfCancellationRequested check (at least before the save) or drop the parameter.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Stale — this was addressed in 4a1d4f0 before this comment's commit was current.
EnsurePoolPathAsync now calls cancellationToken.ThrowIfCancellationRequested() at entry (InstallationCasPoolService.cs:37) and again immediately before the TryUpdateAndSaveAsync call (InstallationCasPoolService.cs:127), which is the point the comment specifically asked for. The parameter is honored, so it stays.
| var configuredCurrentPath = currentSettings.CasConfiguration.InstallationPoolRootPath; | ||
| var currentPath = NormalizePath(configuredCurrentPath); | ||
| var historicalAutoDerivedMarker = | ||
| currentSettings.ExplicitlySetProperties.Contains(ExplicitInstallationPoolPathKey); |
There was a problem hiding this comment.
WARNING: Provenance marker is never populated, so these checks are inert in production
ExplicitInstallationPoolPathKey is nameof(CasConfiguration.InstallationPoolRootPath) (InstallationPoolRootPath), a nested property. UserSettingsService.MarkExplicitlySetPropertiesFromJson only marks top-level UserSettings properties, so ExplicitlySetProperties never contains this key when settings are loaded from JSON. Consequently historicalAutoDerivedMarker (line 75), the IsAutoDerived clause (line 191), the settingsAlreadyMatch clause (line 117), and the ExplicitlySetProperties.Remove call (line 129) are no-ops in production, and the "remove obsolete explicit-setting marker" migration never fires. The unit test masks this by adding the key manually. Either populate the marker for nested properties or drop these dead branches.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
This is incorrect — the marker does get populated in production, so the branches are live and the migration is needed.
The analysis only considered MarkExplicitlySetPropertiesFromJson, which maps top-level camelCase keys through ConvertJsonPropertyNameToCSharp. That is not the only way the set is populated:
ExplicitlySetPropertiesis a plain serializedHashSet<string>onUserSettings(UserSettings.cs:85) with no[JsonIgnore]. It round-trips throughJsonSerializer.Deserialize<UserSettings>directly, so whatever was persisted is restored verbatim — no per-key mapper involved.- The currently shipped code on
developmentwrites exactly this key.ContentOrchestrator.EnsureInstallationPoolPathAsynccallss.MarkAsExplicitlySet(nameof(s.CasConfiguration.InstallationPoolRootPath))on both the single-installation and preferred-installation paths (ContentOrchestrator.cs:753and:777ondevelopment), inside the sameTryUpdateAndSaveAsyncthat persists the settings file.
So any user who has acquired GameClient content on a current build already has "InstallationPoolRootPath" in their persisted explicitlySetProperties. That is precisely the historical auto-derived provenance this PR migrates away — the marker was written by automatic derivation, never by user intent, which is why it is treated as auto-derived rather than as an explicit setting. The unit test adding the key manually reproduces that persisted state rather than masking anything.
Keeping the branches as-is.
| return null; | ||
| } | ||
|
|
||
| if (Path.HasExtension(installationPath)) |
There was a problem hiding this comment.
WARNING: Path.HasExtension misclassifies directories whose name contains a dot
Path.HasExtension returns true for any path whose final segment contains a dot, not only files. A versioned install folder such as C:\Games\ZeroHour v1.04 is treated as a file, so Path.GetDirectoryName strips the real install directory and the CAS pool is derived one level too high (e.g. C:\Games\.genhub-cas instead of C:\Games\ZeroHour v1.04\.genhub-cas). Prefer checking for a known executable extension or Directory.Exists before stripping.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return casPoolPath; | ||
| if (settings.UseInstallationAdjacentStorage) | ||
| { | ||
| var installationPath = Path.HasExtension(installation.InstallationPath) |
There was a problem hiding this comment.
WARNING: Path.HasExtension misclassifies dotted directory names
Same issue as InstallationCasPoolService.GetDerivedPoolPath: a directory like ...\ZeroHour v1.04 is treated as a file because Path.HasExtension is true for the .04 segment, so Path.GetDirectoryName discards the real install folder and the adjacent pool is resolved one level too high.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /// <inheritdoc/> | ||
| public ICasStorage GetStorage(CasPoolType poolType) | ||
| { | ||
| RefreshInstallationPools(); |
There was a problem hiding this comment.
WARNING: Pool refresh now runs on every access under a global lock
GetStorage previously refreshed only for CasPoolType.Installation; it now calls RefreshInstallationPools() for every pool type, and GetAllStorages/EnsureAllPoolsInitialized do the same. RefreshInstallationPools holds _initLock across settings reads and filesystem writability probes, serializing all CAS lookups behind one lock. Probes are cached, but on a cold or slow drive (removable/network) every storage access stalls. Consider skipping the refresh on the common Primary-only path or moving the I/O out of the lock.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return; | ||
| } | ||
|
|
||
| _legacyInstallationStorage = CreateStorage(legacyRoot); |
There was a problem hiding this comment.
WARNING: App-directory security guard is bypassed for the legacy pool
InitializePool rejects CAS roots inside the application directory, but RefreshLegacyInstallationPool builds the legacy storage via CreateStorage(legacyRoot) directly, skipping that guard. legacyRoot comes from LegacyInstallationPoolRootPath/InstallationPoolRootPath settings, and ICasStorage is not read-only (StoreObjectAsync/DeleteObjectAsync are exposed), so a legacy root that resolves into the app directory creates a CAS storage exactly where the guard forbids it. Apply the same AppContext.BaseDirectory check before constructing the legacy storage.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Stale — the guard is already applied to the legacy pool.
RefreshLegacyInstallationPool calls IsInsideApplicationDirectory(legacyRoot) at CasPoolManager.cs:280, before CreateStorage(legacyRoot) is ever reached. When it matches, the legacy storage and root are cleared and a Security Block: error is logged — the same treatment InitializePool gives. No legacy CasStorage can be constructed inside the application directory.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental commit Files Reviewed (8 files)
Previous Review Summaries (5 snapshots, latest commit 84c836d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 84c836d)Status: No Issues Found | Recommendation: Merge The incremental commits ( Files Reviewed (2 files)
Previous review (commit 140f798)Status: No Issues Found | Recommendation: Merge The incremental commits ( Files Reviewed (3 files)
Previous review (commit 4a1d4f0)Status: No Issues Found | Recommendation: Merge The previously reported Files Reviewed (2 files)
Previous review (commit a885079)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous review (commit 7d7098d)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (16 files)
Reviewed by glm-5.2 · Input: 60.8K · Output: 16.3K · Cached: 961.5K |
coderabbitai
Bot
removed
Bug
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs (1)
254-285: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
legacyRootbefore comparing it toactiveInstallationRoot.The equality check at Line 257-259 compares the raw, unnormalized
legacyRootagainstactiveInstallationRoot, and only callsPath.GetFullPath(legacyRoot)afterward at Line 266. If the two paths refer to the same directory but differ in trailing separator or case, the check misses the match. The method then creates a secondICasStoragefor the same directory and logs it as a retained "legacy" pool, even though it is identical to the active writable Installation pool.Move the normalization before the comparison.
🐛 Proposed fix
var legacyRoot = _poolResolver.GetLegacyInstallationPoolRootPath(); - if (string.IsNullOrWhiteSpace(legacyRoot) || - !Directory.Exists(legacyRoot) || - string.Equals(legacyRoot, activeInstallationRoot, PathHelper.PathComparison)) + if (string.IsNullOrWhiteSpace(legacyRoot) || !Directory.Exists(legacyRoot)) { _legacyInstallationStorage = null; _legacyInstallationPoolRoot = null; return; } legacyRoot = Path.GetFullPath(legacyRoot); + var normalizedActiveRoot = string.IsNullOrEmpty(activeInstallationRoot) + ? string.Empty + : Path.GetFullPath(activeInstallationRoot); + if (string.Equals(legacyRoot, normalizedActiveRoot, PathHelper.PathComparison)) + { + _legacyInstallationStorage = null; + _legacyInstallationPoolRoot = null; + return; + } + if (IsInsideApplicationDirectory(legacyRoot))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs` around lines 254 - 285, Update RefreshLegacyInstallationPool to normalize legacyRoot with Path.GetFullPath before comparing it to activeInstallationRoot. Keep the existing invalid-path checks and ensure the normalized path is used for the equality check and subsequent storage setup, preventing retention of the active installation pool as a legacy pool.GenHub/GenHub/Features/Content/Services/ContentOrchestrator.cs (1)
684-712: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
EnsureInstallationPoolPathAsyncwrapper logic across two files. Both methods invalidate the installation cache, fetch installations, fall back to success on discovery failure, and delegate toIInstallationCasPoolService.EnsurePoolPathAsync; the shared root cause is that this wrapper was not centralized when both call sites adopted the new service.
GenHub/GenHub/Features/Content/Services/ContentOrchestrator.cs#L684-L712: extract the invalidate-cache/get-installations/fallback/delegate sequence into a shared helper (either a static extension overIInstallationCasPoolServiceor a new method on the interface, e.g.EnsureEffectivePoolPathAsync(IGameInstallationService, CancellationToken)), and call it here.GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs#L620-L649: replace this method body with a call to the same shared helper, removing the duplicated logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub/Features/Content/Services/ContentOrchestrator.cs` around lines 684 - 712, The installation cache invalidation, installation retrieval, fallback, and pool-path delegation are duplicated across EnsureInstallationPoolPathAsync in GenHub/GenHub/Features/Content/Services/ContentOrchestrator.cs lines 684-712 and GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs lines 620-649. Extract this sequence into one shared helper or IInstallationCasPoolService method, preserving its cancellation, fallback, and error behavior, then replace both method bodies with calls to that helper.GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs (1)
162-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper for deriving the installation-adjacent CAS pool path. Both files independently combine an installation path with
DirectoryNames.GenHubCasPoolto derive the adjacent CAS pool location. Both copies needed the identical dotted-directory-name fix in this same PR, showing the duplication already diverges when only one copy is corrected.
GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs#L162-L178: extractGetDerivedPoolPath's path-selection (InstallationPath/ZeroHourPath/GeneralsPathfallback) plusPath.Combine(..., DirectoryNames.GenHubCasPool)into a shared static helper (for example inPathHelper).GenHub/GenHub/Common/Services/StorageLocationService.cs#L39-L50: call the same shared helper instead of re-derivingPath.Combine(installationPath, DirectoryNames.GenHubCasPool)inline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs` around lines 162 - 178, The installation-adjacent CAS pool path derivation is duplicated and must be centralized. In GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs lines 162-178, move the InstallationPath/ZeroHourPath/GeneralsPath fallback selection and DirectoryNames.GenHubCasPool combination from GetDerivedPoolPath into a shared static helper. In GenHub/GenHub/Common/Services/StorageLocationService.cs lines 39-50, replace the inline Path.Combine derivation with calls to that helper.
♻️ Duplicate comments (2)
GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs (1)
195-217: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
SelectLegacyPathcan silently drop an earlier retained legacy pool.
existingLegacyPathis a single string, andCasConfiguration.LegacyInstallationPoolRootPath(see the referencedCasConfigurationsnippet) stores only one path. When the preferred installation changes more than once,previousPathbecomes the just-supersededcurrentPath, and if that directory still exists, it overwritesexistingLegacyPathoutright.Trace: installation A is active with no legacy pool. The preferred installation changes to B; legacy correctly becomes A. The preferred installation changes again to C;
previousPathis now B, and since B's directory exists, legacy becomes B — A is dropped from tracking even though A's directory and its CAS objects remain on disk and readable. This contradicts the PR objective to preserve existing CAS content without silently orphaning it.Track retained legacy roots as a collection instead of a single string, and append newly discovered roots without discarding previously retained ones. This requires updating
CasConfiguration.LegacyInstallationPoolRootPathand its consumers (for exampleCasPoolResolver.GetLegacyInstallationPoolRootPath, which reads this field as a single string).♻️ Conceptual direction (not a drop-in diff; touches CasConfiguration.cs and its readers)
// CasConfiguration.cs public List<string> LegacyInstallationPoolRootPaths { get; set; } = new();- private static string SelectLegacyPath( - UserSettings settings, - string currentPath, - string candidatePath, - string effectivePath) - { - var existingLegacyPath = NormalizePath(settings.CasConfiguration.LegacyInstallationPoolRootPath); - var previousPath = !string.IsNullOrWhiteSpace(currentPath) - ? currentPath - : candidatePath; - - if (!string.IsNullOrWhiteSpace(effectivePath) && - string.Equals(previousPath, effectivePath, PathHelper.PathComparison)) - { - return existingLegacyPath.Equals(effectivePath, PathHelper.PathComparison) - ? string.Empty - : existingLegacyPath; - } - - return Directory.Exists(previousPath) - ? previousPath - : existingLegacyPath; - } + private static List<string> SelectLegacyPaths( + UserSettings settings, + string currentPath, + string candidatePath, + string effectivePath) + { + var retained = settings.CasConfiguration.LegacyInstallationPoolRootPaths + .Select(NormalizePath) + .Where(path => !string.IsNullOrWhiteSpace(path) && + !string.Equals(path, effectivePath, PathHelper.PathComparison)) + .ToList(); + + var previousPath = !string.IsNullOrWhiteSpace(currentPath) ? currentPath : candidatePath; + if (!string.Equals(previousPath, effectivePath, PathHelper.PathComparison) && + Directory.Exists(previousPath) && + !retained.Contains(previousPath, PathHelper.PathComparer)) + { + retained.Add(previousPath); + } + + return retained; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs` around lines 195 - 217, Replace the single legacy-root value in CasConfiguration with a collection of retained legacy pool paths, then update SelectLegacyPath and all consumers such as CasPoolResolver.GetLegacyInstallationPoolRootPath to append newly superseded existing roots without removing previously retained paths. Preserve path normalization, comparison, and empty-result behavior while ensuring repeated installation changes retain every readable legacy pool.GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/InstallationCasPoolServiceTests.cs (1)
366-371: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Disposestill has no error handling forDirectory.Delete.
Directory.Delete(_tempPath, true)runs unguarded. Several tests in this class (for example lines 200-220, 249-294, 300-332, 338-364) constructCasStorageandCasPoolManagerinstances without disposing them. If any instance holds an open file handle under_tempPathon Windows,Directory.DeletethrowsIOExceptionorUnauthorizedAccessException, and the cleanup failure masks the real test result.This was already flagged in a prior review and remains unresolved in this hunk.
💚 Proposed fix
/// <inheritdoc/> public void Dispose() { - Directory.Delete(_tempPath, true); + try + { + Directory.Delete(_tempPath, true); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or DirectoryNotFoundException) + { + // Temporary directory cleanup must not fail the test run. + } + GC.SuppressFinalize(this); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/InstallationCasPoolServiceTests.cs` around lines 366 - 371, Update Dispose to handle failures from Directory.Delete(_tempPath, true) without allowing cleanup exceptions to mask the test result, while preserving GC.SuppressFinalize(this). Catch the relevant filesystem exceptions around deletion and ensure cleanup remains safe when CasStorage or CasPoolManager instances still hold handles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Common/Services/UserSettingsServiceTests.cs`:
- Around line 135-165: Strengthen
LoadSettings_AfterSave_PreservesInstallationPoolProvenanceMarker by asserting
that loadedSettings.CasConfiguration.InstallationPoolRootPath equals the
explicitly assigned historical installation path, in addition to the existing
ExplicitlySetProperties assertion.
---
Outside diff comments:
In `@GenHub/GenHub/Features/Content/Services/ContentOrchestrator.cs`:
- Around line 684-712: The installation cache invalidation, installation
retrieval, fallback, and pool-path delegation are duplicated across
EnsureInstallationPoolPathAsync in
GenHub/GenHub/Features/Content/Services/ContentOrchestrator.cs lines 684-712 and
GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs
lines 620-649. Extract this sequence into one shared helper or
IInstallationCasPoolService method, preserving its cancellation, fallback, and
error behavior, then replace both method bodies with calls to that helper.
In `@GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs`:
- Around line 254-285: Update RefreshLegacyInstallationPool to normalize
legacyRoot with Path.GetFullPath before comparing it to activeInstallationRoot.
Keep the existing invalid-path checks and ensure the normalized path is used for
the equality check and subsequent storage setup, preventing retention of the
active installation pool as a legacy pool.
In `@GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs`:
- Around line 162-178: The installation-adjacent CAS pool path derivation is
duplicated and must be centralized. In
GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs lines
162-178, move the InstallationPath/ZeroHourPath/GeneralsPath fallback selection
and DirectoryNames.GenHubCasPool combination from GetDerivedPoolPath into a
shared static helper. In GenHub/GenHub/Common/Services/StorageLocationService.cs
lines 39-50, replace the inline Path.Combine derivation with calls to that
helper.
---
Duplicate comments:
In
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/InstallationCasPoolServiceTests.cs`:
- Around line 366-371: Update Dispose to handle failures from
Directory.Delete(_tempPath, true) without allowing cleanup exceptions to mask
the test result, while preserving GC.SuppressFinalize(this). Catch the relevant
filesystem exceptions around deletion and ensure cleanup remains safe when
CasStorage or CasPoolManager instances still hold handles.
In `@GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs`:
- Around line 195-217: Replace the single legacy-root value in CasConfiguration
with a collection of retained legacy pool paths, then update SelectLegacyPath
and all consumers such as CasPoolResolver.GetLegacyInstallationPoolRootPath to
append newly superseded existing roots without removing previously retained
paths. Preserve path normalization, comparison, and empty-result behavior while
ensuring repeated installation changes retain every readable legacy pool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c695858-2b30-436b-8024-6c3edaffa14a
📒 Files selected for processing (15)
GenHub/GenHub.Core/Interfaces/Common/IStorageWritabilityProbe.csGenHub/GenHub.Tests/GenHub.Tests.Core/Common/Services/ConfigurationProviderServiceTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Common/Services/StorageLocationServiceTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Common/Services/UserSettingsServiceTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/ContentOrchestratorTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/CasPoolWritabilityTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/InstallationCasPoolServiceTests.csGenHub/GenHub/Common/Services/ConfigurationProviderService.csGenHub/GenHub/Common/Services/StorageLocationService.csGenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.csGenHub/GenHub/Features/Content/Services/ContentOrchestrator.csGenHub/GenHub/Features/Storage/Services/CasPoolManager.csGenHub/GenHub/Features/Storage/Services/CasService.csGenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.csGenHub/GenHub/Infrastructure/DependencyInjection/ConfigurationModule.cs
💤 Files with no reviewable changes (1)
- GenHub/GenHub/Infrastructure/DependencyInjection/ConfigurationModule.cs
coderabbitai
Bot
added
Bug
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs`:
- Around line 265-269: Update the legacy-root validation in the method
containing legacyRoot and normalizedActiveRoot to also normalize CasRootPath and
reject any legacy root matching either the active installation root or the
primary CasRootPath. Preserve the existing path comparison behavior, and add a
regression test covering LegacyInstallationPoolRootPath equal to CasRootPath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab519121-b8d8-44df-989c-e3859dfceef2
📒 Files selected for processing (3)
GenHub/GenHub.Tests/GenHub.Tests.Core/Common/Services/UserSettingsServiceTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/InstallationCasPoolServiceTests.csGenHub/GenHub/Features/Storage/Services/CasPoolManager.cs
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Went through the open review threads and verified each against the branch. Two were valid and are now fixed; the rest do not survive checking. Fixed
Not applicable
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs (1)
227-248: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse one availability and root snapshot for installation storage.
RefreshInstallationPoolsrecords availability at Line [231], then requests the installation root at Lines [232-234].CasPoolResolver.GetPoolRootPath(CasPoolType.Installation)checks availability again and returns_config.CasRootPathwhen that check fails.If settings or writability changes between these calls,
installationPoolAvailableremainstruewhilecurrentRootbecomes the primary root. Lines [245-248] can then initializeCasPoolType.Installationover the primary CAS pool. This creates duplicate storage with different pool keys and can route installation content to the wrong pool.Return one effective installation-root snapshot from the resolver, or make
InitializePoolreject the primary fallback forCasPoolType.Installation. Add a regression test with a changing probe result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs` around lines 227 - 248, Update RefreshInstallationPools and the installation-root resolution flow to use one consistent availability/root snapshot, preventing CasPoolType.Installation from being initialized with the primary CAS root when availability changes between checks. Prefer returning an effective installation-root snapshot from CasPoolResolver, or make InitializePool reject a primary-root fallback for installation pools; add a regression test covering a changing availability probe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@GenHub/GenHub/Features/Storage/Services/CasPoolManager.cs`:
- Around line 227-248: Update RefreshInstallationPools and the installation-root
resolution flow to use one consistent availability/root snapshot, preventing
CasPoolType.Installation from being initialized with the primary CAS root when
availability changes between checks. Prefer returning an effective
installation-root snapshot from CasPoolResolver, or make InitializePool reject a
primary-root fallback for installation pools; add a regression test covering a
changing availability probe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52a4a1f7-5bb7-4263-b2b8-2ce6876bef4f
📒 Files selected for processing (3)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Storage/InstallationCasPoolServiceTests.csGenHub/GenHub/Features/Storage/Services/CasPoolManager.csGenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs
💤 Files with no reviewable changes (1)
- GenHub/GenHub/Features/Storage/Services/InstallationCasPoolService.cs
coderabbitai
Bot
added
Bug
* fix(cas): fall back from an unwritable installation CAS pool * fix(cas): preserve fallback pool state safely * fix(cas): harden writable pool fallback * fix(cas): honor pool selection cancellation * fix(cas): normalize legacy pool roots * fix(cas): avoid duplicate primary legacy storage * test(cas): tolerate cleanup failures in installation pool tests * refactor(cas): drop a redundant installation-path check * fix(cas): retain every previous installation pool root for lookup (cherry picked from commit b3f5c4a)
1 participant
Summary
Prevent installation-adjacent CAS storage from breaking content acquisition when
the game is installed in a protected location such as Program Files.
GenHub now verifies that it can create and write the actual installation-pool
directory before routing content there. When the location is unavailable, new
content uses the primary user-writable pool while existing readable objects
remain discoverable through a read-only legacy pool.
Changes
location resolution.
failed probe directories safely.
primary pool when installation storage is unavailable.
marker.
without copying or deleting its contents. Roots accumulate rather than replacing
one another, so a pool that moves more than once does not strand the objects
written to an earlier root.
while keeping cached lookups free of initialization locks and filesystem checks.
directories.
primary CAS root.
actual-directory probing, effective display paths, and legacy-content lookup.
Testing
dotnet test GenHub/GenHub.Tests/GenHub.Tests.Core/GenHub.Tests.Core.csproj -c Releasedotnet build GenHub/GenHub.Linux/GenHub.Linux.csproj -c Releasegit diff --checkMSI\bobti; elevation check returnedFalse.0.0.1035-pr349from run30774539724, PR head7fb0a1b.C:\Program Files\EA Games\Command and Conquer Generals Zero Hour; a direct write probe was denied as expected.weekly-2026-07-31GameClient successfully.C:\Users\bobti\AppData\Roaming\GenHub\cas-pool;InstallationPoolRootPathremained empty.C:\Users\bobti\AppData\Local\GenHub\Workspaces..genhub-casor.genhub-workspacedirectories were created.SuperHackers - Generalslaunched successfully from its user-writable workspace.Risks and rollback
workspace materialization; fix(workspace): fall back from protected adjacent storage #346 provides that copy path.
pool is retained only for lookup, while new writes use the effective writable
pool.
prior behavior when absent.
LegacyInstallationPoolRootPathsis a list ratherthan a single path, and has not shipped in any release, so no settings
migration is required.
not enable mutation of legacy or protected pools.
protected-path acquisition failures.
Related issues
Fixes #347
Related to #344
Related to #346
Related to #307
Greptile Summary
The PR makes CAS pool selection tolerate protected installation locations while retaining existing content through read-only legacy pools.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the previously reported dotted-directory pool-selection issue is corrected in both relevant resolution paths and covered by focused tests.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Detect game installations] --> B[Derive installation-adjacent CAS path] B --> C{Writability probe succeeds?} C -->|Yes| D[Persist active installation pool] C -->|No| E[Route new writes to primary pool] D --> F[Write newly acquired content] E --> F D --> G[Retain previous roots as legacy pools] E --> G G --> H[Search active, primary, and legacy pools on reads]Reviews (7): Last reviewed commit: "fix(cas): retain every previous installa..." | Re-trigger Greptile
Context used (3)